Introduce custom TLV support for OnionMessage #2830
Conversation
WalkthroughThe updates involve the integration of Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (4)
- lightning/src/blinded_path/message.rs (4 hunks)
- lightning/src/onion_message/messenger.rs (4 hunks)
- lightning/src/onion_message/packet.rs (2 hunks)
- lightning/src/util/ser_macros.rs (2 hunks)
Additional comments: 13
lightning/src/blinded_path/message.rs (4)
- 25-26: The addition of the
custom_tlvsfield to theForwardTlvsstruct is consistent with the PR's objective to support custom TLVs.- 35-36: The addition of the
custom_tlvsfield to theReceiveTlvsstruct aligns with the PR's goal of extending TLV support.- 67-69: The update to the
blinded_hopsfunction correctly initializescustom_tlvswith an empty vector, which is consistent with the changes made to theForwardTlvsandReceiveTlvsstructs.- 87-87: The changes in the
advance_path_by_onefunction correctly handle thecustom_tlvswhen reading theControlTlvs::Forwardvariant, aligning with the PR's objectives to support custom TLVs.lightning/src/onion_message/packet.rs (2)
- 285-300: The custom TLV decoding logic added to the
ControlTlvsenum is consistent with the PR's objective to support custom TLVs and appears to correctly handle the decoding process.- 312-317: The handling of
custom_tlvsin theControlTlvsenum when constructing the payload format is appropriate and aligns with the changes made to support custom TLVs.lightning/src/onion_message/messenger.rs (5)
- 630-630: The addition of
custom_tlvsto theReceiveControlTlvs::Unblindedvariant aligns with the PR's objective to support custom TLVs.- 635-636: The addition of
custom_tlvsto theForwardControlTlvs::Unblindedvariant is consistent with the changes made toReceiveControlTlvsand supports the PR's goal for custom TLV support.- 1112-1112: The initialization of
custom_tlvswithVec::new()in theForwardTlvsstruct is correct and ensures that the vector is empty by default.- 1123-1123: The addition of
custom_tlvswith an empty vector in theForwardTlvsstruct within theForwardControlTlvs::Unblindedvariant is consistent with the other changes and is correctly implemented.- 1157-1157: The
ReceiveTlvsstruct is correctly updated to includecustom_tlvswith an empty vector, which is in line with the PR's objectives.lightning/src/util/ser_macros.rs (2)
- 475-475: The macro
decode_tlv_stream_with_custom_tlv_decodeis correctly defined and follows the established pattern for TLV stream decoding with the addition of custom TLV handling.- 829-845: The macro
_init_and_read_tlv_stream_with_custom_tlv_decodeis correctly defined and follows the established pattern for initializing and reading a TLV stream with custom TLV decoding.
|
With the current version of PR, I am seeking concept/approach ACKs before I start working on the test suite. A gentle ping, @jkczyz :) |
shaavan
force-pushed
the
#1970/custom-tlvs
branch
from
55098b4
to
c9d57f8
Compare
|
Updated from pr2380.01 to pr2830.02 (diff):
Note: |
|
@shaavan It sounds like you've made significant progress on the PR by addressing previous comments and making adjustments to the |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (8)
- lightning/src/blinded_path/message.rs (3 hunks)
- lightning/src/blinded_path/payment.rs (10 hunks)
- lightning/src/ln/blinded_payment_tests.rs (3 hunks)
- lightning/src/ln/channelmanager.rs (1 hunks)
- lightning/src/ln/msgs.rs (1 hunks)
- lightning/src/onion_message/messenger.rs (2 hunks)
- lightning/src/onion_message/packet.rs (2 hunks)
- lightning/src/util/ser_macros.rs (2 hunks)
Files skipped from review as they are similar to previous changes (4)
- lightning/src/blinded_path/message.rs
- lightning/src/onion_message/messenger.rs
- lightning/src/onion_message/packet.rs
- lightning/src/util/ser_macros.rs
Additional comments: 17
lightning/src/blinded_path/payment.rs (9)
- 19-19: The
Readtrait has been imported fromcrate::io.- 57-58: A
custom_tlvsfield of typeVec<(u64, Vec<u8>)>has been added to theReceiveTlvsstruct to store custom TLV entries.- 127-127: The
Writeabletrait implementation forReceiveTlvshas been updated to include thecustom_tlvsfield with anoptional_vecencoding.- 145-173: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [148-182]
The
Readabletrait implementation forBlindedPaymentTlvshas been modified to decode custom TLVs and populate thecustom_tlvsfield inReceiveTlvs.
- 344-344: The test
compute_payinfohas been updated to include an emptycustom_tlvsvector in theReceiveTlvsstruct.- 363-363: The test
compute_payinfo_1_hophas been updated similarly to include an emptycustom_tlvsvector in theReceiveTlvsstruct.- 417-417: The test
simple_aggregated_htlc_minhas been updated to include an emptycustom_tlvsvector in theReceiveTlvsstruct.- 468-468: The test
aggregated_htlc_minhas been updated to include an emptycustom_tlvsvector in theReceiveTlvsstruct.- 523-523: The test
aggregated_htlc_maxhas been updated to include an emptycustom_tlvsvector in theReceiveTlvsstruct.lightning/src/ln/blinded_payment_tests.rs (3)
- 59-59: The addition of
custom_tlvs: Vec::new()initializes an empty vector for custom TLVs. This is consistent with the PR's objective to introduce custom TLV support. However, it's important to ensure that this new field is actually used somewhere in the code or tests, otherwise it's dead code.Verification successful
The
custom_tlvsfield is actively used throughout the codebase, including in tests, message handling, and payment processing. This confirms that the addition of this field is not dead code and aligns with the PR's objectives.* 93-93: The same `custom_tlvs: Vec::new()` addition here. The comment for the `get_blinded_route_parameters` function applies here as well. Ensure that the `custom_tlvs` field is utilized effectively in the codebase. * 136-136: Again, `custom_tlvs: Vec::new()` is added. The same verification is needed to ensure that this field is not just dead code and is actually being used as intended.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if custom_tlvs are used elsewhere in the codebase rg "custom_tlvs" --type rustLength of output: 11880
Verification successful
The
custom_tlvsfield is actively used throughout the codebase, including in payment processing and onion message handling. The initialization ofcustom_tlvswith an empty vector is consistent with its usage in other parts of the code, where it is later populated and utilized. Therefore, the addition ofcustom_tlvs: Vec::new(),in thempp_to_one_hop_blinded_pathfunction aligns with the intended use of custom TLVs within the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Reusing the previous verification script as it applies here too rg "custom_tlvs" --type rustLength of output: 11880
lightning/src/ln/msgs.rs (3)
- 2400-2401: The
custom_tlvsfield in theReceiveTlvsstruct is being matched to_, which indicates that any value is accepted but not used within the block. Please confirm if this is the intended behavior, as it may be necessary to handlecustom_tlvsexplicitly if they are to be used for processing or validation.- 2401-2401: The use of
unwrap_or(0)fortotal_msatassumes that a missingtotal_msatcan be treated as 0. Please ensure that this assumption is valid in all cases, as it could lead to unexpected behavior iftotal_msatis expected to be provided but is not.- 2401-2401: The error handling for
amt_msatusingok_or(DecodeError::InvalidValue)is appropriate and ensures that a missingamt_msatresults in a decode error.lightning/src/ln/channelmanager.rs (2)
- 7942-7942: The addition of the
custom_tlvsfield initialized with an empty vector seems appropriate for optional or extendable features. Ensure that this field is properly populated and utilized in the downstream logic where the blinded payment paths are created.- 7942-7942: Verify if the
custom_tlvsfield should be passed to thecreate_blinded_payment_pathsfunction. If it is intended to be used for handling custom TLVs within the blinded payment paths, it may need to be included in the function call.
|
@shaavan Great to hear that you've addressed the comments from @TheBlueMatt and made the necessary changes to the Action performedReview triggered. |
|
The are still some conceptual kinks to be ironed out before finalising the custom TLV's implementation. |
shaavan
force-pushed
the
#1970/custom-tlvs
branch
2 times, most recently
from
0cdac04
to
07e73c5
Compare
|
Updated from pr2380.02 to pr2830.03 (diff): Changes:
Note:
I'm still refining the approach for custom TLVs within |
shaavan
force-pushed
the
#1970/custom-tlvs
branch
from
07e73c5
to
2668794
Compare
shaavan
force-pushed
the
#1970/custom-tlvs
branch
from
2668794
to
4c207d4
Compare
- This update lets users add their own custom TLVs to the reply path of a sent onion message. - By including these custom TLVs, users can send custom data with the message and get it back in the response.
- This update introduces a clear separation between two types of custom TLVs: those sent by the sender for the user, and those added by the user to the reply path, which are expected to return with the response. - This commit establishes this distinction in the codebase at relevant points. - The next commit will build on this by providing an interface for users to add their own custom TLVs to the reply path, allowing them to receive specific data back in the response. Note: 1. Similar to keysend, for serialization purposes, user_custom_tlv are assigned a specific TLV number. 2. For uniformity, user_custom_tlv are assigned the lowest possible odd number after (1 << 16), which is 65537.
- Building on the previous commit, this update allows users to include their own custom TLVs within the reply path of a sent onion message. - With this, users can attach custom data to the message, which will be returned in the response, providing more flexibility for custom use cases.
shaavan
force-pushed
the
#1970/custom-tlvs
branch
from
4c207d4
to
f9a8608
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2830 +/- ##
==========================================
- Coverage 89.68% 89.67% -0.01%
==========================================
Files 128 128
Lines 104891 104979 +88
Branches 104891 104979 +88
==========================================
+ Hits 94068 94137 +69
- Misses 8119 8133 +14
- Partials 2704 2709 +5 ☔ View full report in Codecov by Sentry. |
Part of #1970
TODO: